Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Проверка модификации ключей структуры вне функции-конструктора #1054 #1393

Merged
merged 3 commits into from
Dec 22, 2023
Merged

Проверка модификации ключей структуры вне функции-конструктора #1054 #1393

merged 3 commits into from
Dec 22, 2023

Conversation

marmyshev
Copy link
Collaborator

@marmyshev marmyshev commented Dec 20, 2023 •
edited
Loading

Что сделано

  • Проверка модификации ключей структуры вне функции конструктора

Чек-лист

Общее:

  • ветка PR обновлена из master и нет конфликтов
  • Тесты-кейсы, JUnit тесты правильного и неправильного состояния
  • Измененные Вами исходники отформатированы в соответствии с конвенцией
  • Авто-аудит (SonarQube и CheckStyle) пройден, покрытие кода хорошее, ошибок нет, плохой код устранен
  • Добавлена запись в ИСТОРИЮ ИЗМЕНЕНИЯ, включаемая в пользовательскую документацию плагина

Если применимо:

  • Пользовательская документация на доп.инструменты написана (на русском)
  • Описание проверок - на двух языках

Закрываемые задачи

Closes #1054

@marmyshev marmyshev added this to the 0.7 для EDT 2023.3 milestone Dec 20, 2023
@marmyshev marmyshev self-assigned this Dec 20, 2023
@marmyshev marmyshev marked this pull request as draft December 20, 2023 22:57
@marmyshev marmyshev force-pushed the feature/1054-structure-key-modification branch 2 times, most recently from 7fb5f3b to 606fb80 Compare December 21, 2023 21:09
@marmyshev marmyshev force-pushed the feature/1054-structure-key-modification branch from 606fb80 to 1a9994f Compare December 21, 2023 21:12
@marmyshev marmyshev marked this pull request as ready for review December 21, 2023 21:12
iArtemv
iArtemv approved these changes Dec 22, 2023
View reviewed changes
Copy link
Collaborator

@iArtemv iArtemv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Мелкие опечатки записал, в ерп ни одной ошибки не нашлось

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

2 New issues
0 Security Hotspots
83.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@marmyshev marmyshev merged commit dad25b7 into master Dec 22, 2023
4 checks passed
@marmyshev marmyshev deleted the feature/1054-structure-key-modification branch December 22, 2023 21:35
MaksimDzyuba
MaksimDzyuba reviewed Dec 24, 2023
View reviewed changes
....v8codestyle.bsl/src/com/e1c/v8codestyle/bsl/strict/check/StructureKeyModificationCheck.java
}

if (property instanceof DerivedProperty && keyName.equalsIgnoreCase(property.getName())
&& ((DerivedProperty)property).getSource() instanceof BslDerivedPropertySource)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

дешевле сперва проверять на intanceof, чем на equals

MaksimDzyuba
MaksimDzyuba reviewed Dec 24, 2023
View reviewed changes
bundles/com.e1c.v8codestyle.bsl/src/com/e1c/v8codestyle/bsl/strict/check/messages_ru.properties
@@ -84,6 +84,22 @@ StructureCtorValueTypeCheck_description = Проверяет строковый

StructureCtorValueTypeCheck_title = Типизация значений в конструкторе структуры

StructureKeyModificationCheck_Check_Clear_method = Проверять метод Очистить()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

кажется, что скобки тут не нужны, достаточно имя метода просто в кавычки

MaksimDzyuba
MaksimDzyuba reviewed Dec 24, 2023
View reviewed changes
....v8codestyle.bsl/src/com/e1c/v8codestyle/bsl/strict/check/StructureKeyModificationCheck.java
.extension(new StrictTypeAnnotationCheckExtension())
.module()
.checkedObjectType(DYNAMIC_FEATURE_ACCESS)
.parameter(PARAM_CHECK_INSERT, Boolean.class, Boolean.TRUE.toString(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а можно меня просветить, это какой-то стандарт об этом говорит, так хочет сообщество, почему мы сразу все это включаем и собираемся проверять. Если кажется, что так просто правильнее писать код, то надо понимать, на сколько оно затронет текущее решения, сколько новых ошибок они увидят?

MaksimDzyuba
MaksimDzyuba reviewed Dec 24, 2023
View reviewed changes
Copy link
Collaborator

@MaksimDzyuba MaksimDzyuba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

По коду возражений нет, но нужно обоснование включение этого сразу, даже под строгими типами

@marmyshev
Copy link
Collaborator Author

marmyshev commented Dec 25, 2023 •
edited
Loading

@MaksimDzyuba Посмотри пожалуйста, описание проверки - там есть объяснение. Может быть недостаточно избыточное и изобилующее примерами...

Основные причины:

  1. модификация "внешней" структуры - может приводить ошибкам при неокуратном использовании. К тому же можно сменить тип значения для ключа, но исходная сигнатура структуры будет прежней и код в других местах не узнает что тип значения был сменён.
  2. Даже в локальном коде: добавил ключ с типом1 потом заменил ключ на данные с типом2 - это уже проблема, так как в анализе вылезут оба типа, а в рантайме будет какой-то один. Для строгой типизации это как бы проблема. (Но сейчас мы это не запрещаем!)
  3. У нас уже есть проверка которая запрещает ключи структур без типов. Эта проверка - это логическое продолжение.

См. Так же в исходный тикет.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaksimDzyuba MaksimDzyuba MaksimDzyuba left review comments

@iArtemv iArtemv iArtemv approved these changes

@AlmazNasibullin AlmazNasibullin Awaiting requested review from AlmazNasibullin

Assignees

@marmyshev marmyshev

Labels
None yet
Projects
None yet
Milestone
0.7 для EDT 2023.3
3 participants
@marmyshev @iArtemv @MaksimDzyuba